Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reversing Dependency: Implementing Untyped-RPC on Top of Typed-RPC (draft) #51

Closed
wants to merge 16 commits into from

Conversation

mbarbin
Copy link
Contributor

@mbarbin mbarbin commented Jan 6, 2024

In this Pull Request, I've reversed the dependency between the typed-RPC and
untyped-RPC implementations. Now, the untyped-RPC is built on top of the
typed-RPC, a shift from the previous structure.

This change is motivated by several factors:

  • It paves the way for a straightforward deprecation of the untyped-RPC in the
    future, as we can simply remove the code when ready.

  • It reduces indirection and allocation in the typed-RPC implementation path.
    With this new implementation, there's no need to map over sequences, which
    should enhance the performance of the typed implementation and clarify the code.

While this modification isn't strictly necessary for the PR introducing the
typed-RPC interface (#48), I believe it's a valuable addition. This approach aligns
with my original vision during the exploration phase, and I recommend
implementing it without significant delay. Specifically, I suggest that the
typed interface implementation for other concurrency libraries should follow a
similar path, bypassing the initial exploratory phases of building the typed-RPC
on top of the untyped one.

- `Grpc_protobuf` is no longer eio specific
- Make use of ocaml-protoc's server implementation bundle as intended;

- Adapt routeguide example to ocaml-protoc to show the differences.

This results in a more complicated [Grpc.Rpc] interface that tries to
better capture the common parts between ocaml-protoc and
ocaml-protoc-plugin that can be used by ocaml-grpc.
- This adds type safety to ensure the RPCs are called with the
  expected protocol (e.g. you cannot call a unary rpc with a
  server_streaming entry point, etc.).

On the ocaml-protoc-plugin side, currently there are no markers for
the rpc modes - this interface will permit adding them in the future
without user facing changes.

On the ocaml-protoc plugin, the value mode flows from the proto file
definition and is checked in the user code as expected.

Implementation note: There's perhaps a way to shorten the mapping of
value-modes but I couldn't find one given that `Grpc` cannot depend on
`Ocaml_protoc`, and thus the `Value_mode` types are not equal.
In the existing examples, the service name is separated from the
package name by a dot, which I inadvertently omitted in the previous
implementation.

Note that as long as the service name used by a client and a server is
the same, the right handler is executed, so there's some leeway in the
actual choice of the convention to use. The hope is that the dot
separated one is standard.
@mbarbin
Copy link
Contributor Author

mbarbin commented Jan 6, 2024

Currently this PR is built on top of #48 thus duplicating the commit chain.

Also, currently I have only gone through the client side, I plan on making a similar change to the server side shortly.

@mbarbin
Copy link
Contributor Author

mbarbin commented Jan 8, 2024

@quernd and @tmcgilchrist I prefer this implementation over the one in #48. If you like it too, I will use this as the basis for the typed-rpc-eio implementation that I enable for code review. Please let me know what you think. Thank you!

@mbarbin mbarbin changed the title Reversing Dependency: Implementing Untyped-RPC on Top of Typed-RPC Reversing Dependency: Implementing Untyped-RPC on Top of Typed-RPC (draft) Jan 8, 2024
@mbarbin
Copy link
Contributor Author

mbarbin commented Apr 16, 2024

I ended up re-using this code organization in #55.

@mbarbin mbarbin closed this Apr 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant